Sign manifests asynchronously #1265
Conversation
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
9 times, most recently
from
11f03e7 to
61a8af4
Compare
pulp_container/app/tasks/sign.py
Outdated
| manifest_file.write(artifact.file.read()) | ||
| manifest_file.flush() | ||
| async with tempfile.NamedTemporaryFile(dir=".", mode="wb", delete=False) as tf: | ||
| await tf.write(artifact.file.read()) |
There was a problem hiding this comment.
| await tf.write(artifact.file.read()) | |
| await tf.write(await sync_to_async(artifact.file.read)()) |
This should probably not be blocking...
There was a problem hiding this comment.
Based on your latest comment, I am reverting the suggested change:
#1285 (comment)
Can you sign this off?
There was a problem hiding this comment.
Trying to understand what i meant back then. I still think that django storages does not yet know about async. So "artifact.file.read" (which is probably a blocking call,) should not be called from an async context directly, but sync_to_async as i suggested.
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
from
af0bd00 to
5924150
Compare
|
I cannot really test the changes locally because of multiple permission errors in oci_env. Is someone with a vagrant box volunteering to test the changes? Or, do we trust our CI tests? |
|
I can test, my vagrant box seems to be alive. |
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
2 times, most recently
from
ef0872c to
119a70b
Compare
|
can you update this line https://github.com/pulp/pulp_container/pull/1265/files#diff-94c6436dad7eb9a689d63e5b6e6ceb0d18ddac41ce565c1aa91313c1be8ccfc2R35 we sign manifest lists and children manifests are signed by tag too |
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
2 times, most recently
from
37633fa to
ddc64d3
Compare
|
This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution! |
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
from
ddc64d3 to
4cd7531
Compare
|
This pull request is no longer marked for closure. |
|
|
||
| from aiofiles import tempfile |
There was a problem hiding this comment.
Oh... That name is a bit deceiving. But ok.
pulp_container/app/tasks/sign.py
Outdated
| added_signatures.append(signature_pk) | ||
| if tagged_manifest.media_type in MANIFEST_MEDIA_TYPES.LIST: | ||
| # parse ML and sign per-arches | ||
| added_signatures = [] |
There was a problem hiding this comment.
Why are you clearing that list here?
There was a problem hiding this comment.
I should not. Oopsie woopsie. 👀
pulp_container/app/tasks/sign.py
Outdated
| manifest_file.write(artifact.file.read()) | ||
| manifest_file.flush() | ||
| async with tempfile.NamedTemporaryFile(dir=".", mode="wb", delete=False) as tf: | ||
| await tf.write(artifact.file.read()) |
There was a problem hiding this comment.
Trying to understand what i meant back then. I still think that django storages does not yet know about async. So "artifact.file.read" (which is probably a blocking call,) should not be called from an async context directly, but sync_to_async as i suggested.
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
from
4cd7531 to
46076ba
Compare
pulp_container/app/tasks/sign.py
Outdated
| @@ -19,6 +20,10 @@ | |||
| SIGNATURE_TYPE, | |||
| ) | |||
|
|
|||
| SIGNING_TASKS_COUNTER = 10 | |||
There was a problem hiding this comment.
can we expose this to the settings so it is configurable?
I know, this PR then won't be backportable, which is ok given that we don't have now any BZs opened and if we will then backporting without the settings is always an option.
"MAX_PARALLEL_..." |
lubosmj
force-pushed
the
asynchronous-signing-1208
branch
from
46076ba to
761434b
Compare
closes #1208